Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fontique: Improve docs #97

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

waywardmonkeys
Copy link
Contributor

No description provided.

@waywardmonkeys
Copy link
Contributor Author

I've been writing a fontique example and this fell out of doing that ... more to come before I take it out of draft mode. Comments welcome though.

@waywardmonkeys waywardmonkeys force-pushed the fontique-docs branch 2 times, most recently from 417d495 to 66dcf1b Compare July 30, 2024 07:49
fontique/src/attributes.rs Show resolved Hide resolved
fontique/src/font.rs Outdated Show resolved Hide resolved
fontique/src/lib.rs Show resolved Hide resolved
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks good.

A few peripheral details about how linking works.

Comment on lines 100 to 103
/// # See also
///
/// * [`Stretch::from_percentage`]
/// * [`Stretch::ratio`]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in linebender/kurbo#363, I don't think these cross-links to methods on the same type are helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find them very helpful and I think that in some cases, removing them has made these changes worse. When you have something like Collection with a variety of methods doing different things, but with some of the same terms in them, it is useful to have an indication of which are similar sets of functionality.

Instead, someone (probably not me) should write a larger doc comment for Collection that has sections about the functionality, especially fallbacks, and then links to the right methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And since your argument is "the Rust std lib doesn't do them", the next move is clear: Get some better cross-linking into the Rust standard lib.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the reason that these aren't especially helpful is that these methods are all directly next to each other.

That is, the order of the methods is part of the documentation, and that ordering places the constructors first, and all next to each other, then the corresponding interpretation methods.

fontique/src/collection/mod.rs Outdated Show resolved Hide resolved
fontique/src/collection/mod.rs Outdated Show resolved Hide resolved
fontique/src/fallback.rs Outdated Show resolved Hide resolved
Comment on lines 278 to 282
/// * [`FontInfo::has_italic_axis()`]
/// * [`FontInfo::has_optical_size_axis()`]
/// * [`FontInfo::has_slant_axis()`]
/// * [`FontInfo::has_weight_axis()`]
/// * [`FontInfo::has_width_axis()`]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should instead link directly to these at the relevant item in the list above.
With possibly a sentence like:

FontInfo allows cheaply querying for the presence of these axes using the linked methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just removed them instead as I didn't like the other options. I think that's a net negative that this info is gone.

pub fn any(&self) -> bool {
self.len != 0 || self.embolden || self.skew != 0
}

/// Returns the variation settings that should be applied to match the
/// requested attributes.
///
/// When using `parley`, these can be used to create `FontVariation`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe? we should link to docs.rs here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do other crosslinks work on docs.rs? I don't particularly like the idea of absolute links to docs.rs inside the code.

Copy link
Member

@DJMcNab DJMcNab Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is rust-lang/rust#74481
And indeed is similar to https://linebender.org/blog/doc-include/

Ultimately, there are two options:

  • link to docs.rs, and have that be non-ideal behaviour when browsing the docs locally
  • don't link to docs.rs, and have users need to search for the correct type themselves

I think the former is slightly better, but I won't block on this point

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left it as-is for someone else to revisit if they like.

Copy link
Member

@xorgy xorgy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree somewhat with Daniel's point about cross linking within the same type. Otherwise I approve.

@waywardmonkeys
Copy link
Contributor Author

I think all of the review stuff has been addressed.

@xorgy xorgy added the documentation Improvements or additions to documentation label Aug 19, 2024
@waywardmonkeys waywardmonkeys added this pull request to the merge queue Aug 19, 2024
Merged via the queue into linebender:main with commit 4870ff8 Aug 19, 2024
20 checks passed
@waywardmonkeys waywardmonkeys deleted the fontique-docs branch August 19, 2024 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants